New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix: invalid autofix in function-call-argument-newline (fixes #12454) #12539
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for contributing! Can we add tests that document that it does autofix block comments? They can be the same tests tests, just with block comments instead of line comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks for contributing!
@kaicataldo It looks like there are some tests with the "consistent" option which show that the autofix works: eslint/tests/lib/rules/function-call-argument-newline.js Lines 423 to 450 in d679dd8
It may be a good idea to add more tests for other options, but could that potentially be in a separate PR? |
Ah, I missed that. Thanks for pointing that out! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
@@ -390,6 +390,21 @@ ruleTester.run("function-call-argument-newline", rule, { | |||
} | |||
] | |||
}, | |||
{ | |||
code: "fn(a,/* commeent */\nb)", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a blocker, but I’m assuming the intention was comment
rather than commeent
here 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kaicataldo Oh it's typo. I fixed it. Thanks.
What is the purpose of this pull request? (put an "X" next to item)
[ ] Documentation update
[x] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:
What changes did you make? (Give an overview)
This is a PR for #12454
// comment
) before currentArgTokenIs there anything you'd like reviewers to focus on?